KAFKA-19749 The version-mapping of kafka-features.sh should work without requiring the bootstrap
#20619
Conversation
jack2012aa
changed the title
version-mapping of kafka-features.sh should work without requiring the bootstrapversion-mapping of kafka-features.sh should work without requiring the bootstrap
Thanks for the review! |
| // bootstrap_server and bootstrap_controller are in a mutually exclusive group, | ||
| // so the exception happens only when both of them are missing | ||
| throw new ArgumentParserException("one of the arguments --bootstrap-server --bootstrap-controller is required", parser); |
There was a problem hiding this comment.
How about new ArgumentParserException(e.getMessage(), parser)
There was a problem hiding this comment.
Thanks for the review! Yes, it's better to use the existing message rather than write a new one. The code has been updated.
| Namespace namespace = parser.parseArgsOrFail(args); | ||
| Namespace namespace = parser.parseArgs(args); | ||
| String command = namespace.getString("command"); | ||
| if (command.equals("version-mapping")) { |
There was a problem hiding this comment.
This approach allows version-mapping to be used with both bootstrap-server and bootstrap-controller, which seems a bit odd to me
There was a problem hiding this comment.
The version-mapping and feature-dependencies subcommands don't use bootstrap-server and bootstrap-controller arguments at all. The best practice is to let these arguments be required arguments of other subcommands. But I am not sure whether this change will require an KIP or worth one. Do you have any suggestion?
Or, we can check whether user uses these arguments with version-mapping and feature-dependencies, which makes the code more ugly but can provide user more information.
There was a problem hiding this comment.
But I am not sure whether this change will require an KIP or worth one. Do you have any suggestion?
That is a good point. Let's keep this version.
There was a problem hiding this comment.
Alternatively, we could file a KIP to deprecate the config. It shows a warning message in 4.x, but it will stop the tool with an exception in 5.x. What do you think?
Description
In kafka-features.sh, exclusive argument group of
--bootstrap-serverand
--bootstrap-controlleris required in the main parser, butversion-mappingandfeature-dependenciessubcommands don't need theinformation.
Changes
To avoid change in argument level, the exclusive group is not moved. It
is now not required and only check for existence if the subcommand needs
it.
Since
FeatureCommand#mainNoExitcatchesArgumentParserException,ArgumentParser#parseArgsis used instead ofparseArgsOrFail. Itmakes testing easier.
Two tests are added. One tests whether not-remote subcommand can execute
without bootstrap. Another tests whether remote subcommand cannot
execute without bootstrap.
Reviewers: Ken Huang [email protected], Jhen-Yung Hsu
[email protected], TaiJuWu [email protected], Chia-Ping Tsai
[email protected]